PMM-15228 Improve NGINX and Auth server performance - #5658

managed/services/grafana/auth_server.go:406

• The metrics label uses the raw X-Original-Uri header when extractOriginalRequest fails. That header can include query strings and high-cardinality / potentially sensitive values, which is risky to expose in Prometheus labels. Use a stable, safe route label in this error path (e.g., the current req.URL.Path which will be /auth_request).

		s.incAuthRequests(req.Method, req.Header.Get("X-Original-Uri"), http.StatusBadRequest)

managed/services/grafana/auth_server.go:451

• route is recorded as the full cleaned request path (e.g. /graph/api/datasources/proxy/8/ in tests). That can create unbounded label cardinality and an ever-growing sync.Map (memory leak over time) when paths contain IDs or other variable segments. Consider using the matched rule prefix from resolveRule (or another normalized route name) as the route label instead of the raw path.

	s.incAuthRequests(req.Method, req.URL.Path, http.StatusOK)

managed/services/grafana/auth_server.go:258

• mAuthRequests uses the raw cleaned request path as a label and as part of the sync.Map key. For paths with variable segments (e.g. Grafana proxy routes like /graph/api/datasources/proxy//), this can create unbounded time series and unbounded in-process memory growth because entries are never evicted from the sync.Map. Consider normalizing the label (e.g., use the matched rule prefix from resolveRule / nextPrefix chain, or otherwise bucket variable segments) to keep cardinality bounded.

		mAuthRequestsDesc: prom.NewDesc(
			prom.BuildFQName(prometheusNamespace, prometheusSubsystem, "requests_total"),
			"Total number of authentication requests.",
			[]string{"method", "route", "status_code"},
			nil,

managed/services/grafana/auth_server.go:444

• route label is currently set to the full request path (req.URL.Path / X-Original-Uri). That can create unbounded label cardinality (IDs, arbitrary paths) and also grows s.mAuthRequests without bound (one sync.Map entry per unique path/method/status), which can become a memory/DoS risk over time. Consider using the matched rule prefix (from rules/methodRules) or another bounded route identifier for the metric label instead of the raw path.

		status := httpStatusForAuthError(authErr.code)
		s.incAuthRequests(req.Method, req.URL.Path, status)
		s.returnError(rw, status, m, l)

• managed/services/agents/mock_limiter_test.go: Generated file
• managed/services/grafana/mock_access_control_test.go: Generated file

managed/services/grafana/auth_server.go:181

• Typo in comment: "filers" -> "filters".

	// encoded filers to be added as proxy headers.

managed/services/qan/client.go:184

• Typo in comment: "preasure" -> "pressure".
  managed/services/grafana/auth_server.go:172
• Typo in comment: "validiness" -> "validity" (and "TTL" is typically capitalized).

This issue also appears on line 181 of the same file.

	// Ttl for auth response validiness in auth cache.

build/ansible/roles/nginx/files/conf.d/pmm.conf:76

• Comment contradicts the actual keys_zone=STATIC:1m value: it says 10 MB, but 1m is 1 MB.

  # keys_zone=STATIC:1m - Allocates a 10 MB area in RAM called STATIC.

build/ansible/roles/nginx/files/conf.d/pmm.conf:87

• Comment contradicts the actual max_size=128m value: it says the max disk footprint is 10M.

  # Sets up a 1MB memory zone named 'AUTH_CACHE' and a max disk footprint of 10M.

utils/cache/cache_ttl_test.go:16

• File header mixes Apache-2.0 licensing text with an AGPL notice, which is internally inconsistent and can confuse license checks.
  utils/cache/cache_ttl_bench_test.go:16
• File header mixes Apache-2.0 licensing text with an AGPL notice, which is internally inconsistent and can confuse license checks.
  utils/rateLimiter/concurrencyLimiter.go:62
• Release() always increments availableSlots, so calling Release more times than TryAcquire permanently increases capacity beyond the configured max, which defeats the stated purpose of "limiting" concurrency. If this is intentional, the type/docs should reflect that; otherwise consider tracking maxSlots and clamping (or panicking) on over-release.

utils/rateLimiter/concurrencyLimiter.go-39-61 (1)

39-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Preserve the configured concurrency limit.

An unmatched Release can create slots above maxSlots. This disables the configured concurrency protection. Store the configured maximum and prevent Release from increasing available slots above it.

• utils/rateLimiter/concurrencyLimiter.go#L39-L61: retain maxSlots and bound Release.
• utils/rateLimiter/concurrencyLimiter_test.go#L71-L83: replace the release-before-acquire expectation with an assertion that capacity does not exceed the configured limit.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/rateLimiter/concurrencyLimiter.go` around lines 39 - 61, Update
utils/rateLimiter/concurrencyLimiter.go lines 39-61: have ConcurrencyLimiter
retain maxSlots when NewConcurrencyLimiter initializes it, and bound Release so
availableSlots never exceeds that configured maximum. Update
utils/rateLimiter/concurrencyLimiter_test.go lines 71-83 by replacing the
release-before-acquire expectation with an assertion that capacity remains
capped at maxSlots.

utils/rateLimiter/concurrencyLimiter_bench_test.go-22-55 (1)

22-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use testify assertions in the rate-limiter tests

Replace direct Fatal assertions with require or assert in both affected files. The repository includes testify, and nearby utils tests use it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/rateLimiter/concurrencyLimiter_bench_test.go` around lines 22 - 55,
Replace direct Fatal-based assertions in
BenchmarkConcurrencyLimiter_TryAcquireRelease and
BenchmarkConcurrencyLimiter_TryAcquireWhenExhausted in
utils/rateLimiter/concurrencyLimiter_bench_test.go, and the corresponding
assertions in utils/rateLimiter/concurrencyLimiter_test.go lines 23-110, with
testify require or assert calls; add or reuse the appropriate testify import
while preserving each test’s existing expectations.

Source: Coding guidelines

utils/cache/cache_ttl_bench_test.go-15-16 (1)

15-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the AGPL distribution notice from the Apache-2.0 headers.

These files declare Apache-2.0 terms and also state that the program includes an AGPL license copy. Use the Apache-2.0 Percona header only.

• utils/cache/cache_ttl_bench_test.go#L15-L16: remove the AGPL distribution notice.
• utils/cache/cache_ttl_test.go#L15-L16: remove the AGPL distribution notice.

Based on learnings, “Go files under the repository-root directories agent/, admin/, and utils/ must use the Apache-2.0 Percona license header.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/cache/cache_ttl_bench_test.go` around lines 15 - 16, Remove the AGPL
distribution notice from the Apache-2.0 Percona headers in
utils/cache/cache_ttl_bench_test.go lines 15-16 and
utils/cache/cache_ttl_test.go lines 15-16, leaving only the standard Apache-2.0
Percona license header for both Go test files.

Source: Learnings

utils/cache/cache_test.go-21-143 (1)

21-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use Testify assertions consistently.

Replace direct t.Fatal and t.Fatalf assertions with require for preconditions and assert for comparisons.

• utils/cache/cache_test.go#L21-L143: import and use Testify assertion helpers.
• utils/cache/cache_ttl_test.go#L27-L205: replace direct assertions with Testify assertion helpers.

As per coding guidelines, “Use testify/assert and testify/require; do not use testify suites.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/cache/cache_test.go` around lines 21 - 143, Replace direct t.Fatal and
t.Fatalf assertions in utils/cache/cache_test.go lines 21-143 and
utils/cache/cache_ttl_test.go lines 27-205 with testify/assert and
testify/require helpers. Use require for setup or precondition checks and assert
for value comparisons, importing the helpers consistently while preserving each
test’s existing expectations.

Source: Coding guidelines

managed/services/victoriametrics/victoriametrics.go-485-485 (1)

485-485: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Move the comment onto its own line.

The comment sits on the closing brace of the error check. It describes the skipExternalExporter assignment on the next line, not the brace.

✏️ Proposed fix

 	settings, err := models.GetSettings(q)
 	if err != nil {
 		return nil, err
-	} // In HA mode, skip ExternalExporter agents if this node is not the leader
+	}
+
+	// In HA mode, skip ExternalExporter agents if this node is not the leader.
 	skipExternalExporter := !svc.haService.IsLeader()

As per coding guidelines: "Do not use inline comments such as code // comment; place comments on separate lines."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/victoriametrics/victoriametrics.go` at line 485, Move the
HA-mode comment currently trailing the closing brace onto its own line
immediately before the skipExternalExporter assignment it describes, leaving the
error-check closing brace unannotated.

Source: Coding guidelines

managed/services/agents/registry.go-252-255 (1)

252-255: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Return a gRPC status code, and parse the version once.

authenticate at Line 348 already parses md.Version and returns codes.InvalidArgument on failure. This second parse repeats the work. If it were reached, it would return a plain error, which the gRPC layer maps to codes.Unknown. The same malformed input would then produce two different codes.

Return the parsed version from authenticate alongside the node, or return a status error here.

🐛 Minimal correction

 	pmmAgentVersion, err := version.Parse(agentMD.Version)
 	if err != nil {
-		return zero, fmt.Errorf("failed to parse PMM agent version %q: %w", agentMD.Version, err)
+		return zero, status.Errorf(codes.InvalidArgument, "Can't parse 'version' for pmm-agent with ID %q.", agentMD.ID)
 	}

As per coding guidelines: "Use status.Error() with proper gRPC status codes for API errors, rather than ad-hoc HTTP errors in service layers."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry.go` around lines 252 - 255, Update the
authentication flow around authenticate and the PMM agent version parsing so
md.Version is parsed only once and the parsed version is reused when
constructing the node. Propagate the parsed version alongside the authenticated
node, or convert this failure to a gRPC status.Error with codes.InvalidArgument,
ensuring malformed versions consistently return that status instead of
codes.Unknown.

Source: Coding guidelines

managed/cmd/pmm-managed/main.go-1086-1090 (1)

1086-1090: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Keep the API pool budgets within capacity.

pmmAgentsConnectionsLimiter allows 70% and stateUpdateRateLimiter allows 80% of apiDbMaxOpenConns. These independent limits allow 150 operations for a 100-connection pool. Use one shared budget, or set both caps so their sum is at most 100%. Change “reserve” to “cap”. The multiple state-update queries are sequential, so do not count them as simultaneous connections.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/cmd/pmm-managed/main.go` around lines 1086 - 1090, Update the API
connection limits used by agents.NewStateUpdater and pmmAgentsConnectionsLimiter
so their combined caps never exceed apiDbMaxOpenConns, while treating sequential
state-update queries as non-concurrent. Change the state-updater comment from
“reserve” to “cap” and use either a shared budget or complementary percentages
totaling at most 100%.

managed/services/realtimeanalytics/service.go-539-544 (1)

539-544: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the validation warning structured.

Use l.WithError(err).Warn(...) instead of formatting err into the message.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/realtimeanalytics/service.go` around lines 539 - 544, Update
the agent validation warning in the FindAgentByID error path to use
l.WithError(err).Warn with a descriptive message, rather than interpolating err
via Warnf. Preserve the existing disconnect behavior and InvalidArgument
response.

Source: Coding guidelines

build/ansible/roles/nginx/files/conf.d/pmm.conf-68-89 (1)

68-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The comments contradict the directives.

Two mismatches exist in this block:

• Line 76 states "Allocates a 10 MB area in RAM called STATIC". Line 85 declares keys_zone=STATIC:1m.
• Line 87 states "a max disk footprint of 10M". Line 89 declares max_size=128m.

Correct the comments so an operator sizing the cache is not misled.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/ansible/roles/nginx/files/conf.d/pmm.conf` around lines 68 - 89,
Correct the cache sizing comments in the nginx configuration: update the STATIC
keys_zone description to state 1 MB, matching keys_zone=STATIC:1m, and update
the AUTH_CACHE disk-footprint description to state 128 MB, matching
max_size=128m. Leave the directives unchanged.

managed/services/grafana/auth_server.go-463-466 (1)

463-466: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Engage the component logger here.

Line 464 calls the package-level logrus.Errorf. The surrounding code uses the *logrus.Entry field s.l. Use s.l so the entry keeps the component field.

🔧 Proposed fix

 	if len(roles) == 0 {
-		logrus.Errorf("User %d has no roles", userID)
+		s.l.Errorf("User %d has no roles", userID)
 		return nil, fmt.Errorf("user %d has no roles", userID)
 	}

As per coding guidelines: "Use structured logging, such as s.l.WithField("key", value).Error("message"), and pass *logrus.Entry rather than *logrus.Logger."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 463 - 466, Update the
no-roles branch in the surrounding service method to replace the package-level
logrus.Errorf call with the component logger entry s.l, preserving the existing
message and error return while retaining the entry’s structured component field.

Source: Coding guidelines

managed/services/grafana/helpers_test.go-176-199 (1)

176-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

One case in TestNextPrefix asserts nothing.

Line 186 holds a single-element slice. The inner loop iterates paths[:len(paths)-1], which is empty for that entry. The subtest runs and passes without any assertion. Add the expected chain, or remove the entry.

💚 Proposed fix

-		{"/v1/server/AWSInstanceCheck/..%2f..%2finventory/Services/List'"},
+		{"/v1/server/AWSInstanceCheck/..%2f..%2finventory/Services/List'", "/v1/server/AWSInstanceCheck/..%2f..%2finventory/Services/"},

Confirm the expected value against the nextPrefix chain before you apply it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_test.go` around lines 176 - 199, Update the
single-element
`"/v1/server/AWSInstanceCheck/..%2f..%2finventory/Services/List'"` entry in
`TestNextPrefix` so it contains the complete expected `nextPrefix` chain,
confirming each value against the implementation; alternatively remove the entry
if no chain is intended. Ensure every test case produces at least one assertion.

managed/cmd/pmm-managed/main.go (2)

1211-1211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer the reform handle for this settings lookup.

sqlInternalDB is the raw *sql.DB. The call compiles, but it bypasses the reform query logger and the Prometheus instrumentation registered at Line 940. It also carries no context. Line 602 already uses deps.db.WithContext(ctx) for the same operation.

♻️ Proposed change

-	settings, err := models.GetSettings(sqlInternalDB)
+	settings, err := models.GetSettings(internalDB.WithContext(ctx))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/cmd/pmm-managed/main.go` at line 1211, Update the settings lookup
around models.GetSettings to use the reform database handle with the current
context, matching the existing deps.db.WithContext(ctx) pattern instead of
passing raw sqlInternalDB. Preserve the existing error handling and settings
flow.

---

906-971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make it so: factor the duplicated pool parameters into one helper.

The two models.SetupDBParams literals repeat fourteen identical fields. Only MaxIdleConns and MaxOpenConns differ. Two copies can drift apart when a field is added later.

Two smaller matters travel with this change. The panic messages at Line 931 and Line 967 are identical, so a log entry cannot identify which pool failed. The comment at Line 909 contains a typo, "serviceses", and a stray tab character.

♻️ Proposed consolidation

Add a helper near the other main.go helpers:

func newDBParams(maxIdle, maxOpen int32) models.SetupDBParams {
	return models.SetupDBParams{
		Address:         *postgresAddrF,
		Name:            *postgresDBNameF,
		Username:        *postgresDBUsernameF,
		Password:        *postgresDBPasswordF,
		SSLMode:         *postgresSSLModeF,
		SSLCAPath:       *postgresSSLCAPathF,
		SSLKeyPath:      *postgresSSLKeyPathF,
		SSLCertPath:     *postgresSSLCertPathF,
		HANodeID:        *haNodeID,
		HAPeers:         nodes,
		ConnMaxLifetime: dbMaxLifeTime,
		ConnMaxIdleTime: dbMaxIdleTime,
		MaxIdleConns:    maxIdle,
		MaxOpenConns:    maxOpen,
	}
}

Then apply this diff:

-	// are still able to communicate with DB and perform tasks to keep system alive
+	// are still able to communicate with DB and perform tasks to keep the system alive
 	// (like update caches, fetch settings, run cleanup tasks, etc).
-	setupInternalDBParams := models.SetupDBParams{
-		Address:         *postgresAddrF,
-		...
-	}
+	setupInternalDBParams := newDBParams(internalDbMaxIdleConns, internalDbMaxOpenConns)
 
 	sqlInternalDB, err := models.OpenDB(setupInternalDBParams)
 	if err != nil {
-		l.Panicf("Failed to connect to database: %+v", err)
+		l.Panicf("Failed to connect to the internal database pool: %+v", err)
 	}

Apply the equivalent change for the API pool with l.Panicf("Failed to connect to the API database pool: %+v", err).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/cmd/pmm-managed/main.go` around lines 906 - 971, Extract the
duplicated database configuration into a newDBParams helper accepting maxIdle
and maxOpen, and use it for both setupInternalDBParams and setupAPIDBParams
while preserving their pool-specific limits. Differentiate the failure messages
so the internal pool uses its own context and the API pool reports “Failed to
connect to the API database pool”. Correct the “serviceses” typo and remove the
stray tab in the internal DB comment.

managed/services/agents/registry_test.go (2)

120-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a concurrency test for repeated kicks.

These tests exercise the sequential paths well. No test drives two Kick calls for the same agent at the same time. That is the exact scenario in which the non-atomic read and delete in registry.go at Line 378 panics.

A test that launches several goroutines against one agent and runs under -race would guard the fix.

💚 Suggested test

func TestRegistryKickIsSafeUnderConcurrentCalls(t *testing.T) {
	t.Parallel()

	r := NewRegistry(nil, fakeVictoriaMetricsParams{}, &fakeHAService{params: &models.HAParams{Enabled: false}})
	r.agentsCache.Set("agent-1", pmmAgentInfo{id: "agent-1", kickChan: make(chan struct{})})
	ctx := logger.Set(context.Background(), "test-request")

	var wg sync.WaitGroup
	for range 16 {
		wg.Go(func() {
			r.Kick(ctx, "agent-1")
		})
	}
	wg.Wait()

	assert.EqualValues(t, 0, r.agentsCache.Size())
}

As per coding guidelines: "Ensure every goroutine has a context- or errgroup-tied exit and does not leak during shutdown; run race tests for concurrency-sensitive packages."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry_test.go` around lines 120 - 165, Add a
concurrency test alongside TestRegistryKickRemovesAgentAndClosesKickChannel that
launches multiple goroutines calling Registry.Kick for the same agent, waits for
all calls to finish, and verifies the agent is removed without panic or race
under -race. Use a synchronization mechanism compatible with the repository’s
conventions and ensure every goroutine has a bounded, context- or errgroup-tied
completion path.

Source: Coding guidelines

---

167-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer generated mocks for these two stubs.

mockery is already configured for this package. The stack lists .mockery.yaml and managed/services/agents/mock_limiter_test.go. Generated mocks track interface changes automatically and support expectation assertions.

Add haService and victoriaMetricsParams to .mockery.yaml and replace these hand-written stubs.

As per coding guidelines: "Use testify/assert and testify/require; do not use testify suites. Generate mocks with mockery rather than routinely hand-rolling fakes."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry_test.go` around lines 167 - 191, Add
haService and victoriaMetricsParams to the configured mockery interfaces in
.mockery.yaml, generate their mocks alongside mock_limiter_test.go, and update
the affected tests to use the generated mocks instead of fakeHAService and
fakeVictoriaMetricsParams. Preserve the existing interface behavior and use mock
expectations where applicable.

Source: Coding guidelines

managed/services/agents/state_test.go (1)

89-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend coverage to the new control paths.

These tests are precise, and the SQL expectations correctly reflect the non-transactional path in UpdateAgentsState.

Four new behaviors in state.go carry no tests:

• runStateChangeHandler batching and its exit on kickChan and on context cancellation.
• The stateUpdateRateLimiter rejection path and the errStateUpdateLimitExceeded branch.
• The backoff retry and reset behavior.
• The singleflight deduplication in sendSetStateRequest.

Every test here passes maxConcurrentUpdates of 1, so the limiter never rejects. A test that sets the limit to 1 and drives two concurrent updates would cover the rejection branch.

I can draft these tests if that would help. Shall I proceed?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/state_test.go` around lines 89 - 127, Extend state
updater tests beyond TestUpdateAgentsStateQueuesUpdatesForAllConnectedAgents to
cover runStateChangeHandler batching and exits on kickChan/context cancellation,
stateUpdateRateLimiter rejection including errStateUpdateLimitExceeded, backoff
retry and reset behavior, and sendSetStateRequest singleflight deduplication.
Use concurrent updates with maxConcurrentUpdates set to 1 to exercise limiter
rejection, and retain SQL expectations for the non-transactional
UpdateAgentsState path.

managed/services/agents/state.go (1)

288-330: 🚀 Performance & Scalability | 🔵 Trivial

Consider batching the per-row service lookups.

The pre-fetched node and the embedded agent version remove substantial query volume. That is a clear gain.

One pattern remains. models.FindServiceByID runs once per row inside this loop, and models.FindNodeByID runs once per RDS exporter. A pmm-agent that monitors fifty services issues fifty sequential queries.

The whole function now runs under the 5-second stateChangeTimeout set in runStateChangeHandler at Line 191. If a large agent exceeds that budget, the request fails, the backoff triggers, and the entire set is retried from the beginning. Under load such an agent may never complete a state update.

Collect the service IDs in a first pass, then resolve them with one filtered query into a map. Apply the same approach to the RDS node lookups.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/state.go` around lines 288 - 330, Refactor the
state-building flow around the agent iteration to collect all service IDs and
RDS node IDs first, resolve them with batched filtered queries, and index the
results by ID. Update the AzureDatabaseExporterType and RDSExporterType branches
to reuse those maps instead of calling models.FindServiceByID or
models.FindNodeByID per row, while preserving existing lookup errors and
configuration behavior.

managed/services/grafana/helpers_bench_test.go (2)

27-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the commented-out code, Ensign.

Lines 33-35 contain a commented-out verification block. The loop below already verifies the result. Delete the dead lines.

🧹 Proposed cleanup

 	b.ReportAllocs()
 
-	// cleanedPath, err := cleanPath(unescapedURI)
-	// require.NoError(b, err)
-	// require.Equal(b, expectedCleanPath, cleanedPath)
-
 	b.ResetTimer()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_bench_test.go` around lines 27 - 47, Remove
the commented-out cleanPath verification block in BenchmarkCleanPath, including
the commented require.NoError and require.Equal lines, while leaving the active
benchmark loop and its validations unchanged.

---

97-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid changing the standard logger for the whole package.

logrus.SetOutput(io.Discard) mutates global state. Other tests and benchmarks in package grafana share that logger. Create a local logger instead.

♻️ Proposed change

-	logrus.SetOutput(io.Discard)
-	l := logrus.NewEntry(logrus.StandardLogger())
+	logger := logrus.New()
+	logger.SetOutput(io.Discard)
+	l := logrus.NewEntry(logger)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_bench_test.go` around lines 97 - 101, Update
BenchmarkResolveRule to stop mutating the global standard logger via
logrus.SetOutput; create a local logrus.Logger configured to discard output,
then build the log entry from that local logger while preserving the benchmark’s
existing behavior.

managed/services/grafana/helpers_test.go (1)

264-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The local variable tests shadows the imported package tests.

The file imports github.com/percona/pmm/managed/utils/tests and uses it at line 191. Line 266 declares a local slice named tests. The code compiles because the scopes differ, but the name reuse is confusing. Rename the local variable.

♻️ Proposed rename

-	tests := []struct {
+	testCases := []struct {
 		path     string
 		expected string
 		wantErr  bool
 	}{

Rename the loop at line 321 as well.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_test.go` around lines 264 - 270, Rename the
local test-case slice `tests` in `TestCleanPath` to avoid shadowing the imported
`tests` package, and update the associated loop at line 321 to use the new name
consistently.

managed/services/grafana/auth_server_bench_test.go (1)

118-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

b.ReportAllocs on line 118 does not apply to the subtests.

Each b.Run receives a new *testing.B. Allocation reporting does not inherit from the parent. Move the call inside the subtest.

♻️ Proposed change

-	b.ReportAllocs()
-
 	for _, tc := range []struct {
@@
 		b.Run(tc.name, func(b *testing.B) {
+			b.ReportAllocs()
 			tokenSeq := 0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_bench_test.go` around lines 118 - 131,
Move b.ReportAllocs() from the parent benchmark into the b.Run subtest callback
within the benchmark table loop, so allocation reporting applies to each
subtest.

managed/services/grafana/auth_server_test.go (2)

203-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

These two mocks serve no purpose.

Lines 205-211 create c and ac and register cleanup assertions. Line 213 then calls setupLBACServer(t), which builds its own mocks. Neither c nor ac is attached to the server under test. Delete them.

🧹 Proposed cleanup

 	t.Run("enabled LBAC - lbacPrefixes", func(t *testing.T) {
 		t.Parallel()
-		c := newMockGrafanaAuthUserGetter(t)
-		ac := newMockAccessControl(t)
-		ac.On("isEnabled").Return(true).Maybe()
-		t.Cleanup(func() {
-			c.AssertExpectations(t)
-			ac.AssertExpectations(t)
-		})
 
 		s, _, _ := setupLBACServer(t)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_test.go` around lines 203 - 220, Remove
the unused c and ac mock setup, including their expectation cleanup and related
isEnabled expectation, from the “enabled LBAC - lbacPrefixes” test; keep
setupLBACServer(t) as the sole server initialization before exercising
needAddLBACFilters.

---

511-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore or remove the commented-out cache assertions.

Line 525 holds a commented-out assertion. The same pattern appears at lines 801, 818, and 836. The helper cacheSize(s) on line 87 gives the working equivalent. Either use it or delete the comments.

🧹 Proposed change for line 525

-		// assert.True(t, len(s.cache) == 0, "cache should be empty on anonymous user")
+		assert.Zero(t, cacheSize(s), "cache should be empty on anonymous user")

Verify the expected value first. getAuthUser caches every positive Grafana reply, including one for an anonymous user, so the count may not be zero.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_test.go` around lines 511 - 527, Remove
the commented-out cache assertions in the anonymous-user test and the matching
cases around the other referenced tests, or restore them using the cacheSize(s)
helper. Verify the expected cache count first, since authenticateUser caches
successful getAuthUser responses even for anonymous users.

managed/services/grafana/helpers.go (1)

244-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the duplicated header extraction, Number One.

extractAuthHeaders and getAuthCacheKey contain the same 8-line block that reads Authorization and Cookie. A third copy exists in AuthServer.getAuthUser in managed/services/grafana/auth_server.go at lines 571-578. Extract one helper and call it from all three sites.

♻️ Proposed refactor

+// authHeaderValues returns the Authorization and Cookie header values.
+func authHeaderValues(req *http.Request) (string, string) {
+	var authorization, cookie string
+	if vals := req.Header["Authorization"]; len(vals) > 0 {
+		authorization = vals[0]
+	}
+	if vals := req.Header["Cookie"]; len(vals) > 0 {
+		cookie = vals[0]
+	}
+	return authorization, cookie
+}
+
 // extractAuthHeaders extracts auth info from request.
 func extractAuthHeaders(req *http.Request) http.Header {
-	// Marginally faster than req.Header.Get("...")
-	var authorization, cookie string
-	if vals := req.Header["Authorization"]; len(vals) > 0 {
-		authorization = vals[0]
-	}
-	if vals := req.Header["Cookie"]; len(vals) > 0 {
-		cookie = vals[0]
-	}
+	authorization, cookie := authHeaderValues(req)
 
 	// Fast path: no auth headers -> no map allocation.
 	if authorization == "" && cookie == "" {
@@
 // getAuthCacheKey returns cache key directly from request auth headers.
 func getAuthCacheKey(req *http.Request) string {
-	// Marginally faster than req.Header.Get("...")
-	var authorization, cookie string
-	if vals := req.Header["Authorization"]; len(vals) > 0 {
-		authorization = vals[0]
-	}
-	if vals := req.Header["Cookie"]; len(vals) > 0 {
-		cookie = vals[0]
-	}
-
+	authorization, cookie := authHeaderValues(req)
 	return authorization + ":" + cookie
 }

The helper is inlinable, so the allocation profile stays the same. Confirm this with the existing BenchmarkAuthCacheKey.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers.go` around lines 244 - 282, Extract the
shared Authorization and Cookie lookup into one inlinable helper near
extractAuthHeaders, returning both values without allocating. Update
extractAuthHeaders, getAuthCacheKey, and AuthServer.getAuthUser to call this
helper, preserving their existing behavior and output; confirm the existing
BenchmarkAuthCacheKey remains allocation-free.

managed/services/grafana/auth_server.go (2)

568-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

A point of order on the cache key.

getAuthCacheKey returns the raw credentials joined by :. The comment on line 584 states that the header comparison protects against "rare hash collisions". The key is not a hash, so two distinct credential pairs can only collide if authorization + ":" + cookie is ambiguous, for example ("a:b", "") versus ("a", "b"). The stored-header comparison does catch that case, so the behavior is correct. Update the comment so it describes the real mechanism.

The raw credential is also used as the singleflight key and as the TTL-cache key. Confirm that no code path logs or exports these keys.

📝 Proposed comment fix

-		// Verify auth headers for this hash to prevent serving wrong user on rare hash collisions.
+		// The cache key concatenates both headers, so verify the stored values
+		// to prevent serving the wrong user on an ambiguous concatenation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 568 - 642, Update the
cache-hit comment in getAuthUser to describe validation against ambiguous raw
credential keys rather than rare hash collisions, while preserving the existing
authorization and cookie comparison. Inspect getAuthCacheKey and all uses of
authCacheKey, including the singleflight and cache paths, to confirm the raw
credentials are never logged or exported; avoid adding changes unless such
exposure is found.

---

239-243: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff

Make it so: return the error instead of a panic.

NewAuthServer panics when NewCacheTTL fails. The constructor is called from main.go wiring, where an error return gives a controlled shutdown. The failure modes of NewCacheTTL are a nil context or a non-positive interval, so this path is unlikely today. A returned error keeps the contract safe against future changes to the constants.

♻️ Proposed signature change

-func NewAuthServer(ctx context.Context, c grafanaAuthUserGetter, db *reform.DB) *AuthServer {
-	cache, err := cache.NewCacheTTL[cachedAuthUser](ctx, cacheItemTTL, cacheInvalidationInterval)
-	if err != nil {
-		panic(err)
-	}
+func NewAuthServer(ctx context.Context, c grafanaAuthUserGetter, db *reform.DB) (*AuthServer, error) {
+	cache, err := cache.NewCacheTTL[cachedAuthUser](ctx, cacheItemTTL, cacheInvalidationInterval)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create auth cache: %w", err)
+	}

Note: this changes the callers in main.go and in the tests and benchmarks.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 239 - 243, Change
NewAuthServer to return (*AuthServer, error) instead of panicking when
cache.NewCacheTTL fails; return the initialization error immediately and return
the server with a nil error on success. Update all callers in main.go, tests,
and benchmarks to handle the constructor error explicitly.

build/ansible/roles/nginx/files/conf.d/pmm.conf (1)

370-379: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Removing the body-size limit opens a resource risk.

Line 377 sets client_max_body_size 0 for the whole /prometheus prefix, which disables the limit for every method and sub-path. The stated goal is large remote_write payloads. Restrict the change to the ingestion path so the rest of the prefix keeps the 10m server limit.

🔒 Proposed narrowing

     location ^~ /prometheus {
       proxy_pass http://victoriametrics;
       proxy_read_timeout 600;
       proxy_http_version 1.1;
       proxy_set_header Connection "";
 
-      # Disable body size limits for large remote_write payloads
-      client_max_body_size 0;
+      # Large remote_write payloads still need a bound.
+      client_max_body_size 512m;
       client_body_buffer_size 10m;
     }

Choose the bound from the largest expected remote_write batch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/ansible/roles/nginx/files/conf.d/pmm.conf` around lines 370 - 379,
Restrict the unlimited body-size configuration in the /prometheus location to
the remote_write ingestion path only, rather than applying client_max_body_size
0 to the entire prefix. Preserve the existing 10m server limit for other methods
and sub-paths, and set the ingestion limit according to the largest expected
remote_write batch.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

managed/services/agents/registry.go (1)

92-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the field detail to a separate comment line.

Line 93 has an inline comment. Place id -> info in the preceding documentation comment.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry.go` around lines 92 - 93, Move the “id ->
info” detail from the inline comment on agentsCache to the preceding
documentation comment, leaving the field declaration without an inline comment.

Source: Coding guidelines

managed/services/grafana/helpers_bench_test.go (2)

31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the commented-out verification block.

Lines 33 to 35 hold commented-out require calls. The file does not import testify, so the lines cannot be restored as written. The loop body at lines 39 to 45 already performs the same check with b.Fatalf.

♻️ Proposed cleanup

 	b.ReportAllocs()
 
-	// cleanedPath, err := cleanPath(unescapedURI)
-	// require.NoError(b, err)
-	// require.Equal(b, expectedCleanPath, cleanedPath)
-
 	b.ResetTimer()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_bench_test.go` around lines 31 - 37, Remove
the commented-out cleanPath verification block between b.ReportAllocs() and
b.ResetTimer() in the benchmark, leaving the existing b.Fatalf-based validation
in the loop unchanged.

---

97-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Do not mutate the global logrus logger from a benchmark.

Line 100 calls logrus.SetOutput(io.Discard) on the standard logger and never restores it. go test runs tests and benchmarks in one process, so this silences logging for every other test in the package. The effect depends on execution order, which makes failures hard to reproduce.

Use a logger instance scoped to this benchmark instead.

♻️ Proposed change

 func BenchmarkResolveRule(b *testing.B) {
-	b.ReportAllocs()
-
-	logrus.SetOutput(io.Discard)
-	l := logrus.NewEntry(logrus.StandardLogger())
+	logger := logrus.New()
+	logger.SetOutput(io.Discard)
+	l := logrus.NewEntry(logger)
+
 	for _, tc := range []struct {
 		name   string
 		method string
 		path   string
 	}{
@@
 		b.Run(tc.name, func(b *testing.B) {
+			b.ReportAllocs()
 			for b.Loop() {
 				_, _ = resolveRule(tc.method, tc.path, l)
 			}
 		})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_bench_test.go` around lines 97 - 101, Remove
the global logrus.StandardLogger mutation in BenchmarkResolveRule and configure
a benchmark-scoped logger instance instead. Update the logger setup used to
create l so its output is discarded without affecting other tests or benchmarks.

build/ansible/roles/nginx/files/conf.d/pmm.conf (3)

247-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct the misleading comment on the assets location.

Line 267 states that the assets are dynamic. Lines 274 and 275 then set expires 30d and Cache-Control "public, max-age=2592000, immutable". Those directives describe immutable static assets, which is the opposite of dynamic.

The likely intent is that the asset filenames are content-hashed, so each file never changes. State that instead, because a future reader may otherwise remove the caching headers as inconsistent.

📝 Proposed comment correction

-    # All PMM UI assets are dynamic - bypass authentication and cache on browser side.
+    # PMM UI asset filenames are content-hashed, so each file is immutable.
+    # Bypass authentication and allow long-lived browser caching.
     location ^~ /pmm-ui/assets/ {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/ansible/roles/nginx/files/conf.d/pmm.conf` around lines 247 - 307,
Update the comment above the /pmm-ui/assets/ location to state that the assets
use content-hashed filenames and are immutable static files, consistent with the
30-day expiration and immutable Cache-Control directives. Leave the caching
configuration unchanged.

---

326-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

expires and add_header Cache-Control together emit two Cache-Control header lines.

Line 335 sets expires 30d, which makes NGINX emit Cache-Control: max-age=2592000. Line 336 then adds a second Cache-Control: public, no-transform line. The response therefore carries two separate Cache-Control headers, and the directives are split across them. Intermediate caches vary in how they merge such headers.

Set one complete value instead.

Line 343 also omits the always flag on add_header X-Cache-Status, so the debug header disappears on error responses. Line 277 uses always for the equivalent header. Align the two for consistent diagnostics.

♻️ Proposed change

-      # Add caching headers to further reduce container load
-      expires 30d;
-      add_header Cache-Control "public, no-transform";
+      # Add caching headers to further reduce container load
+      expires 30d;
+      add_header Cache-Control "public, max-age=2592000, no-transform" always;
@@
-      add_header X-Cache-Status $upstream_cache_status;
+      add_header X-Cache-Status $upstream_cache_status always;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/ansible/roles/nginx/files/conf.d/pmm.conf` around lines 326 - 352,
Update the Grafana static-assets location to emit a single complete
Cache-Control header instead of combining expires with add_header Cache-Control,
preserving the intended 30-day public caching and no-transform directives. Also
update the X-Cache-Status add_header in this location to use the always flag,
matching the equivalent configuration near line 277.

---

87-89: 🚀 Performance & Scalability | 🔵 Trivial

Consider the key capacity of AUTH_CACHE against the agent fleet size.

AUTH_CACHE:1m holds roughly 8,000 keys, per the estimate at line 78. The cache key at line 219 is "$http_authorization|$request_method|$request_uri", so each agent credential consumes one entry for the write endpoint.

For a deployment with more distinct agent credentials than the zone capacity, NGINX evicts entries under LRU pressure. The authentication cache then misses often, and the load returns to pmm-managed. That result is the opposite of the goal stated at lines 399 to 401.

Add a metric or alert on $upstream_cache_status for this location so eviction pressure is visible, and document the fleet size at which the zone requires enlargement.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/ansible/roles/nginx/files/conf.d/pmm.conf` around lines 87 - 89, Add
monitoring for $upstream_cache_status in the auth-cache location, exposing or
alerting on cache misses/evictions so LRU pressure is visible. Update the nearby
AUTH_CACHE documentation to state the approximate credential capacity and the
agent-fleet size at which the 1m zone must be enlarged.

managed/services/grafana/helpers_test.go (1)

176-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

One table row asserts nothing.

The loop at line 190 iterates paths[:len(paths)-1], which treats the final element as the expected result of the previous one. The row at line 186 holds a single element, so the slice is empty. That row runs no assertion and does not reach tests.AddToFuzzCorpus.

Either add the expected prefix chain for that path or delete the row, so the coverage matches what the table appears to declare.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers_test.go` around lines 176 - 199, Update the
single-element test row beginning with "/v1/server/AWSInstanceCheck/.." in
TestNextPrefix so it declares the expected nextPrefix result, or remove the row
if no assertion is intended. Ensure the row produces at least one assertion and
calls tests.AddToFuzzCorpus for the path.

managed/services/grafana/auth_server_bench_test.go (1)

103-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

b.ReportAllocs on the parent benchmark does not apply to the sub-benchmarks.

Line 118 calls b.ReportAllocs() on the outer *testing.B. The b.Run calls that follow create separate *testing.B values, and each must enable allocation reporting itself. As written, the per-route benchmarks report no allocation counts.

This PR targets heap-allocation reduction on the authentication path, so the allocation numbers are the primary signal here.

♻️ Proposed change

 	grafanaMock.On("getAuthUser", mock.Anything, mock.Anything, mock.Anything).
 		Return(authUser{role: admin, userID: 1001}, nil)
 
-	b.ReportAllocs()
-
 	for _, tc := range []struct {
 		name   string
 		method string
 		path   string
 	}{
@@
 		b.Run(tc.name, func(b *testing.B) {
+			b.ReportAllocs()
 			tokenSeq := 0
 			for b.Loop() {

The same placement occurs in managed/services/grafana/helpers_bench_test.go.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_bench_test.go` around lines 103 - 119,
Move allocation reporting from the parent benchmarks to each sub-benchmark
created by b.Run in BenchmarkAuthServerServeHTTP and the corresponding benchmark
in helpers_bench_test.go. Call ReportAllocs on each sub-benchmark’s *testing.B
so every per-route benchmark reports allocation counts.

managed/services/grafana/auth_server.go (3)

499-512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a lower log level for expected client authentication and authorization failures.

Lines 501 and 509 log at Error level. Both conditions are routine client outcomes: an expired token produces the first, and an insufficient role produces the second. This handler runs on every authenticated request, so invalid credentials from one misconfigured agent can fill the log with Error records and mask genuine server faults.

Reserve Error for server-side faults, such as errStaticAuthErrorInternalError. Use Warn or Debug for denials.

♻️ Proposed change to the log levels

 	user, err := s.authenticateUser(req, l) //nolint:contextcheck
 	if err != nil {
-		l.WithError(err).Error("Failed to authenticate user.")
+		l.WithError(err).Warn("Failed to authenticate user.")
 		var zero authResult
 		return zero, err
 	}
 
 	l = l.WithField("role", user.role.String())
 	err = authorizeUser(minRole, user, l)
 	if err != nil {
-		l.WithError(err).Error("Failed to authorize user.")
+		l.WithError(err).Warn("Failed to authorize user.")
 		var zero authResult
 		return zero, err
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 499 - 512, Lower the
log levels for the expected failure paths in the authentication handler: change
the logging for authenticateUser failures and authorizeUser denials from Error
to Warn or Debug, while preserving Error for server-side faults such as
errStaticAuthErrorInternalError.

---

463-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the component logger instead of the package-level logrus logger.

Replacing the panic with an error return is a solid improvement, Number One. One detail remains: line 464 calls logrus.Errorf on the standard logger. That call drops the component field carried by s.l, so these records lose correlation with the rest of the auth component.

The coding guidelines require structured logging through a *logrus.Entry.

♻️ Proposed change to use the structured entry

 	if len(roles) == 0 {
-		logrus.Errorf("User %d has no roles", userID)
+		s.l.WithField("user_id", userID).Error("User has no roles.")
 		return nil, fmt.Errorf("user %d has no roles", userID)
 	}

As per coding guidelines: "Use structured logrus logging with *logrus.Entry, such as s.l.WithField(...).Error(...)".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 463 - 466, Update the
no-roles branch in the relevant auth server method to replace the package-level
logrus.Errorf call with the component logger entry s.l, preserving the existing
message and error return while retaining structured component fields.

Source: Coding guidelines

---

580-627: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Use an unambiguous cache key separator, and re-verify credentials on the singleflight result.

Engage, but with one course correction. Two related weaknesses share a root cause: the key format.

getAuthCacheKey in managed/services/grafana/helpers.go returns authorization + ":" + cookie. The : character occurs inside both header values, so distinct credential pairs can produce one identical key. For example, Authorization: "A:B" with Cookie: "C" and Authorization: "A" with Cookie: "B:C" both yield A:B:C.

The cache path handles this. Lines 585 and 600 compare the stored authorization and cookie against the request values, so a colliding entry falls through to the cold path.

The singleflight path does not. Line 596 uses the same ambiguous string as the singleflight key. Line 611 authenticates with extractAuthHeaders(req) from the closure of whichever caller became the leader. A waiter that collided on the key receives the leader's authUser at line 634 with no comparison against its own headers. The waiter then proceeds with another identity and role.

Exploitation requires prior knowledge of the target credentials, so this is not an authentication bypass. It is still a correctness defect on the authorization path, and one delimiter change removes the whole class.

🔒 Proposed fix: length-prefixed key plus a result re-check

In managed/services/grafana/helpers.go:

 // getAuthCacheKey returns cache key directly from request auth headers.
 func getAuthCacheKey(req *http.Request) string {
 	// Marginally faster than req.Header.Get("...")
 	var authorization, cookie string
 	if vals := req.Header["Authorization"]; len(vals) > 0 {
 		authorization = vals[0]
 	}
 	if vals := req.Header["Cookie"]; len(vals) > 0 {
 		cookie = vals[0]
 	}
 
-	return authorization + ":" + cookie
+	// Length-prefix the first field so the boundary is unambiguous.
+	// "A:B" + "C" and "A" + "B:C" must not produce one key.
+	var b strings.Builder
+	b.Grow(len(authorization) + len(cookie) + 12) //nolint:mnd
+	b.WriteString(strconv.Itoa(len(authorization)))
+	b.WriteByte(':')
+	b.WriteString(authorization)
+	b.WriteString(cookie)
+	return b.String()
 }

In managed/services/grafana/auth_server.go, re-check the deduplicated result:

 	user, ok := res.(authUser)
 	if !ok {
 		l.WithField("type", fmt.Sprintf("%T", res)).Error("Unexpected Grafana user result type.")
 		var zero authUser
 		return zero, errStaticAuthErrorInternalError
 	}
+
+	// The singleflight leader authenticated with its own headers. Confirm the
+	// cached entry it stored matches this request's credentials before use.
+	if cached, found := s.cache.Load(authCacheKey); found {
+		if cached.authorization != authorization || cached.cookie != cookie {
+			l.Error("Auth cache key collision detected; rejecting deduplicated result.")
+			var zero authUser
+			return zero, errStaticAuthErrorInternalError
+		}
+	}
 
 	return user, nil

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 580 - 627, Update
getAuthCacheKey to use an unambiguous, length-prefixed encoding of authorization
and cookie instead of concatenating them with “:”. In the authUserGroup.Do
result handling, re-validate the returned user against the current request’s
authorization and cookie before returning it; if they do not match, do not
accept the deduplicated result and ensure the request is authenticated with its
own credentials.

managed/services/grafana/helpers.go (1)

258-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the duplicated header reads into one helper.

The same block that reads req.Header["Authorization"][0] and req.Header["Cookie"][0], including the identical comment, appears three times:

• extractAuthHeaders, lines 261 to 267.
• getAuthCacheKey, lines 287 to 293.
• AuthServer.getAuthUser in managed/services/grafana/auth_server.go, lines 572 to 578.

getAuthUser calls both helpers and then repeats the reads a third time for the collision check. One small accessor removes all three copies and keeps the zero-allocation property.

♻️ Proposed helper

+// authCredentials returns the raw Authorization and Cookie header values.
+// Direct map access is marginally faster than req.Header.Get.
+func authCredentials(req *http.Request) (string, string) {
+	var authorization, cookie string
+	if vals := req.Header["Authorization"]; len(vals) > 0 {
+		authorization = vals[0]
+	}
+	if vals := req.Header["Cookie"]; len(vals) > 0 {
+		cookie = vals[0]
+	}
+	return authorization, cookie
+}
+
 // extractAuthHeaders extracts auth info from request.
 func extractAuthHeaders(req *http.Request) http.Header {
-	// Marginally faster than req.Header.Get("...")
-	var authorization, cookie string
-	if vals := req.Header["Authorization"]; len(vals) > 0 {
-		authorization = vals[0]
-	}
-	if vals := req.Header["Cookie"]; len(vals) > 0 {
-		cookie = vals[0]
-	}
+	authorization, cookie := authCredentials(req)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/helpers.go` around lines 258 - 296, Extract the
repeated Authorization and Cookie header reads into a shared accessor, then
update extractAuthHeaders, getAuthCacheKey, and AuthServer.getAuthUser—including
its collision check—to reuse it. Preserve the current first-value selection,
empty-header behavior, and zero-allocation fast path.

managed/services/grafana/auth_server_test.go (2)

511-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore or delete the commented-out cache assertions.

Four subtests end with a commented-out assertion that uses the old map-based cache field:

• Line 525 in TestAuthServerAuthenticateUser.
• Line 801 in TestAuthServerProcessRequest, subtest "access forbidden for anonymous user".
• Line 818 in the same test, subtest "access granted for anonymous user".
• Line 836 in the same test, subtest "access granted for anonymous user with LBAC enabled".

Each states that the cache must stay empty for an anonymous user. That behavior is unverified. The cache is now populated by getAuthUser for any successful Grafana lookup, including one that returns userID: 0, so the stated expectation may no longer hold.

This file already provides the cacheSize helper. Either assert with it or delete the comments.

♻️ Proposed change for line 525

 		got, err := s.authenticateUser(req, l)
 		require.NoError(t, err)
 		assert.Equal(t, userInfo, got)
-		// assert.True(t, len(s.cache) == 0, "cache should be empty on anonymous user")
+		assert.Equal(t, int64(1), cacheSize(s), "anonymous lookups are cached like any other")

Confirm the intended caching behavior for anonymous identities, then apply the matching assertion at all four sites.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_test.go` around lines 511 - 526, Confirm
the intended caching behavior for anonymous identities, then update all four
commented-out cache checks in TestAuthServerAuthenticateUser and
TestAuthServerProcessRequest to use the existing cacheSize helper with the
matching expected value, or remove the assertions if anonymous lookups are
intentionally cached; do not leave the stale commented assertions.

---

203-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the unused mocks in this subtest.

Lines 205 to 211 create c and ac, register ac.On("isEnabled"), and add a cleanup that asserts both. Line 213 then calls setupLBACServer(t), which builds its own mocks and assigns them to the server. c and ac are never attached to s, so the assertions verify nothing. The .Maybe() qualifier keeps AssertExpectations passing, which hides the fact that the mocks are inert.

♻️ Proposed cleanup

 	t.Run("enabled LBAC - lbacPrefixes", func(t *testing.T) {
 		t.Parallel()
-		c := newMockGrafanaAuthUserGetter(t)
-		ac := newMockAccessControl(t)
-		ac.On("isEnabled").Return(true).Maybe()
-		t.Cleanup(func() {
-			c.AssertExpectations(t)
-			ac.AssertExpectations(t)
-		})
 
 		s, _, _ := setupLBACServer(t)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_test.go` around lines 203 - 219, Remove
the unused c and ac mock setup, expectation registration, and cleanup from the
“enabled LBAC - lbacPrefixes” subtest; rely on setupLBACServer(t) to configure
the mocks used by s, while preserving the existing prefix assertions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

utils/cache/cache_ttl_test.go (1)

45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use Testify assertions in the three named files.

Replace all direct t.Fatal* and b.Fatal* calls with require or assert. The cache files already import require; add it to managed/services/grafana/helpers_bench_test.go.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/cache/cache_ttl_test.go` around lines 45 - 47, The direct fatal
assertion in utils/cache/cache_ttl_test.go:45-47 should use the existing Testify
require import; replace all t.Fatal* and b.Fatal* calls in
utils/cache/cache_ttl_test.go:45-47 and
utils/cache/cache_ttl_bench_test.go:40-44 with appropriate require or assert
calls. In managed/services/grafana/helpers_bench_test.go:39-45, make the same
replacements and add the Testify require import.

Source: Coding guidelines

managed/services/grafana/auth_server_test.go (1)

203-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the mocks that serve no duty.

Lines 205-211 create c and ac, register an isEnabled expectation, and assert expectations in cleanup. The subtest then calls setupLBACServer(t), which builds its own server and its own mocks. The local mocks are never attached to anything, so they only mislead the reader.

♻️ Proposed cleanup

 	t.Run("enabled LBAC - lbacPrefixes", func(t *testing.T) {
 		t.Parallel()
-		c := newMockGrafanaAuthUserGetter(t)
-		ac := newMockAccessControl(t)
-		ac.On("isEnabled").Return(true).Maybe()
-		t.Cleanup(func() {
-			c.AssertExpectations(t)
-			ac.AssertExpectations(t)
-		})
 
 		s, _, _ := setupLBACServer(t)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server_test.go` around lines 203 - 220, Remove
the unused c and ac mock declarations, their isEnabled expectation, and the
cleanup assertion block from the “enabled LBAC - lbacPrefixes” test; leave
setupLBACServer(t) and the prefix assertions unchanged.

managed/services/grafana/auth_server.go (1)

239-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Return cache initialization errors from NewAuthServer.

Change the constructor to return (*AuthServer, error) and wrap the cache error. Update main.go, auth_server_test.go, and auth_server_bench_test.go to handle it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/grafana/auth_server.go` around lines 239 - 243, Update
NewAuthServer to return (*AuthServer, error), wrapping and propagating failures
from cache.NewCacheTTL instead of panicking, and return the initialized server
with a nil error on success. Adjust callers in main.go, auth_server_test.go, and
auth_server_bench_test.go to handle the constructor’s error result.

Source: Coding guidelines

utils/cache/cache.go (1)

44-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Make it so: use the existing key hash for shard selection.

maphash.String already produces the required 64-bit hash. Return c.shards[keyHash&shardMask] and apply the same change to TTLCache.getShard to avoid the second hash on every operation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/cache/cache.go` around lines 44 - 52, Update Cache.getShard to index
c.shards directly with keyHash&shardMask instead of calling maphash.Comparable.
Apply the same direct shard selection in TTLCache.getShard, preserving the
existing precomputed hash flow.

managed/services/agents/registry_test.go (1)

195-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer generated mocks over hand-written fakes here.

fakeHAService and fakeVictoriaMetricsParams duplicate what mockery produces from the interfaces in managed/services/agents/deps.go. This pull request already updates .mockery.yaml. Generated mocks stay in step with interface changes; hand-written fakes do not.

Add haService and victoriaMetricsParams to the mockery configuration and use the generated types.

As per coding guidelines: "Generate mocks with mockery rather than routinely hand-rolling fakes."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry_test.go` around lines 195 - 219, Replace the
hand-written fakeHAService and fakeVictoriaMetricsParams types in the registry
tests with mockery-generated mocks. Add haService and victoriaMetricsParams to
the mockery configuration, regenerate the mocks from the interfaces in deps.go,
and update test references to use the generated types.

Source: Coding guidelines

managed/services/agents/registry.go (1)

537-545: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Collect sends to a channel while it holds a shard lock.

IterAll holds each shard's read lock for the whole iteration of that shard. The four ch <- sends inside the loop block when the Prometheus consumer is slow. While a send blocks, no goroutine can register, unregister, or kick an agent in that shard.

Snapshot the metrics first, then send outside the iteration.

♻️ Proposed refactor

-	for _, agent := range r.agentsCache.IterAll() {
-		m := agent.channel.Metrics()
-
-		ch <- prom.MustNewConstMetric(mSentDesc, prom.CounterValue, m.Sent, agent.id)
-		ch <- prom.MustNewConstMetric(mRecvDesc, prom.CounterValue, m.Recv, agent.id)
-		ch <- prom.MustNewConstMetric(mResponsesDesc, prom.GaugeValue, m.Responses, agent.id)
-		ch <- prom.MustNewConstMetric(mRequestsDesc, prom.GaugeValue, m.Requests, agent.id)
-	}
+	type agentMetrics struct {
+		id string
+		m  channel.Metrics
+	}
+	snapshot := make([]agentMetrics, 0, r.agentsCache.Size())
+	for _, agent := range r.agentsCache.IterAll() {
+		snapshot = append(snapshot, agentMetrics{id: agent.id, m: agent.channel.Metrics()})
+	}
+	for _, a := range snapshot {
+		ch <- prom.MustNewConstMetric(mSentDesc, prom.CounterValue, a.m.Sent, a.id)
+		ch <- prom.MustNewConstMetric(mRecvDesc, prom.CounterValue, a.m.Recv, a.id)
+		ch <- prom.MustNewConstMetric(mResponsesDesc, prom.GaugeValue, a.m.Responses, a.id)
+		ch <- prom.MustNewConstMetric(mRequestsDesc, prom.GaugeValue, a.m.Requests, a.id)
+	}

Adjust the element type to the concrete return type of channel.Channel.Metrics().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@managed/services/agents/registry.go` around lines 537 - 545, Update
Registry.Collect to snapshot each agent’s metrics and identifier into a
collection while iterating with agentsCache.IterAll, then perform all
prom.Metric channel sends after iteration completes. Use the concrete return
type of channel.Channel.Metrics() for the snapshot element type, ensuring no ch
send occurs while the shard read lock is held.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes